You may want to check if you’re actually in the body of the function, since this fails to filter out e.g. parameter defaults:

const fn = (p = /* 'fn' appears here */) => {}

That said, leaving in some bad completions that we already have is not a big problem IMO, so this is a minor nitpick.

A similar comment applies to isVariableDeclaration(node): what you’re interested in is whether we walked up from the initializer of a variable declaration, but you might have a false positive on a case like this:

const { a, b = /**/ } = { ... }

We’re not interested in getting the variable declaration if we started at the default binding for b. But, it ends up having no effect on the outcome, since there will be no symbol where symbol.valueDeclaration === variableDeclaration. But if it’s easy to skip that property access and equality check, we should do it, since it runs for every completion item. It could maybe shave off a couple milliseconds if the list is long.

This also brings up another point worth noting, that this logic won’t work for destructuring:

const { a, b } = { /** }; // 'a' and 'b' are offered

but I don’t think it’s worth doing anything about this. I really don’t want to have to loop through every binding element declaration for every symbol—I’m happy that currently the check is only one equality comparison per symbol.

1. You may want to check if you’re actually in the body of the function, since this fails to filter out e.g. parameter defaults:

   Yes. You are right, maybe we should use isFunctionBlock to check.

2. We’re not interested in getting the variable declaration if we started at the default binding for b. But, it ends up having no effect on the outcome, since there will be no symbol where symbol.valueDeclaration === variableDeclaration. But if it’s easy to skip that property access and equality check, we should do it, since it runs for every completion item. It could maybe shave off a couple milliseconds if the list is long.

   I think we can use the isBindingPattern to filter out this two situations

const { a, b = /**/ } = { ... }
const [ a, b = /**/ ] = [ ... ]

3. This also brings up another point worth noting, that this logic won’t work for destructuring

   I have the same idea with you. I think the time loss may be too much

For the 1 and 2, I have updated the code and added some test cases. Please help check it @andrewbranch

This doesn’t work for arrow function expressions without braces, e.g. () => expression/**/. It seems like there should be a util for this but I can’t find an existing one: isFunctionBlock(node) || isArrowFunction(node.parent) && node.parent.expression === node

Yes. But I found no node.parent.expression. maybe node.parent.body?
isFunctionBlock(node) || (isArrowFunction(node.parent) && node.parent.body === node)
I has updated the code and add a new test case of arrow function expressions without braces

Yeah, body is what I meant 👍

This was a very strange test

This test has existed before, after this modification, there should be no 'x'. Let me optimize it

I’m not saying there’s anything wrong here, just observing that it was weird for someone to write this test this way in the first place, a long time ago. I think the changes you made to these make sense 👍

shouldn't this be as VariableDeclaration | undefined?

Yes. You are right. Done